1049768 - [TSF] Glitches in Chinese IME with intl.tsf.enable=true

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[wmchan4]

Attachment: FF34-freecj.jpg

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:34.0) Gecko/20100101 Firefox/34.0 (Beta/Release)
Build ID: 20140805170718

Steps to reproduce:

* You need a Traditional Chinese version of Windows 7 to install FreeCJ.
(1) Set intl.tsf.enable=true in FF Nightly;
(2) Download and install FreeCJ from http://input.foruto.com/freecj/. 
Direct download link: http://input.foruto.com/cgi-bin/Counter/download3.cgi;
(3) Open FF, go to a text area, e.g. www.pastebin.com;
(4) Type some Chinese texts. In my case I just entered "m", Space, "m", Space, "m", Space.... (without double quotes)


Actual results:

See Box (1) in the attachment.
You can see glitches (extra spaces), and the first Chinese character is not entered
at all. Also the character selection menu is not seen completely.


Expected results:

See Box (2) in the attachement. 
No extra spaces, the characters are entered responsively.
You also see the character menu with possible candidates for certain inputs.

FF31 isn't affected. 
In Nightly the glitch can be worked around by setting intl.tsf.enable=false.

Related discussion in http://forums.mozillazine.org/viewtopic.php?f=23&t=2858387

Question: Will TSF be enforced permanently in the foreseeable future? 
It will be a disaster for me.

Comment 1

[wmchan4]

Attachment: FF34-ecj.jpg

With EasyChangJei (http://www.ecj.hk/index.html) it also shows another type of glitches.
In this case I type 

"m", Comma, Space, "m", Space

Extra double-byte Underscore appeared when it shouldn't happen.

Setting intl.tsf.enable to false also works around it.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

Thank you for the report. I'll take a look today.

(In reply to wmchan4 from comment #0)
> Question: Will TSF be enforced permanently in the foreseeable future? 
> It will be a disaster for me.

Of cause, yes but not soon.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

Do you know how many users to use the FreeCJ? And also EasyChangJie?

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

Hmm, not showing candidate list window may be a bug of FreeCJ.

It tries to retrieve text position with GetTextExt() after inserting "　". However, it always fails since we've not calculate the layout including the new text yet. At this time, we return TS_E_NOLAYOUT for conforming TSF spec. However, due to a bug of TSF, TIP (FreeCJ) will get E_FAIL. For avoiding this bug, TIP should check if ITfContextView::GetRangeFromPoint() returns TS_E_NOLAYOUT when ITextStore::GetTextExt() returns E_FAIL. If it returns TS_E_NOLAYOUT, TIP should wait a call of ITextStoreACPSink::OnLayoutChange(). We need to contact them if this is true.

I'm still investigating other issues.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

I'll fix only the unexpected commit string in this bug.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

Attachment: If OnCompositionUpdate() never called with new range, we should call RecordCompositionUpdateAction() forcibly before flushing pending actions

The cause of this bug is, these TIPs don't call OnUpdateComposition() with proper range.

If OnUpdateComposition() is never called with new range, we should forcibly record the composition update action before flushing the pending actions.

Comment 7

[Masatoshi Kimura [:emk]]

Comment on attachment 8468975 [details] [diff] [review]
If OnCompositionUpdate() never called with new range, we should call RecordCompositionUpdateAction() forcibly before flushing pending actions

Review of attachment 8468975 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/nsTextStore.cpp
@@ +2935,5 @@
>    }
>  
>    // pRangeNew is null when the update is not complete
>    if (!pRangeNew) {
> +    PendingAction* action = GetPendingCompositionUpdate();

It was not obvious that this function will never return null. It should have been named as something like LastOrNewPendingCompositionUpdate().

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

(In reply to Masatoshi Kimura [:emk] from comment #7)
> Comment on attachment 8468975 [details] [diff] [review]
> > +    PendingAction* action = GetPendingCompositionUpdate();
> 
> It was not obvious that this function will never return null. It should have
> been named as something like LastOrNewPendingCompositionUpdate().

Indeed. Thanks!

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1f01cae60e26

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1f01cae60e26